feat: polymorphic helper for request body streaming#477
feat: polymorphic helper for request body streaming#477tank-bohr wants to merge 1 commit intoelixir-mint:mainfrom
Conversation
|
@ericmj Could you please take a look |
|
What if you make @type Mint.HTTP.t() :: Mint.HTTP1.t() | Mint.HTTP2.t()so that the "top-level" type is not opaque, but the inner types are? |
@whatyouhide That's exactly what my PR does. It un-opaques (exposes) the Mint.HTTP.t(). This will help to solve the dialyzer issue in the existing code (like k8s). Because existing code has to know about internals. And |
|
the original example with the get_send_window will look like defmodule MintDialyzerRepro do
def run(method, %URI{scheme: scheme} = uri, headers, body, opts)
when scheme in ~w[http https] do
scheme = String.to_existing_atom(scheme)
port = uri.port || URI.default_port(uri.scheme)
path = URI.to_string(%URI{uri | scheme: nil, host: nil, authority: nil})
with {:ok, conn} <- Mint.HTTP.connect(scheme, uri.host, port, opts) do
stream(conn, method, path, headers, body)
end
end
defp stream(conn, method, path, headers, body) do
with {:ok, conn, ref} <- Mint.HTTP.request(conn, method, path, headers, :stream) do
stream_body(conn, ref, body)
end
end
defp stream_body(conn, ref, "") do
Mint.HTTP.stream_request_body(conn, ref, :eof)
end
defp stream_body(conn, ref, body) do
chunk_size = Mint.HTTP.next_body_chunk_size(conn, ref, body)
<<chunk::binary-size(chunk_size), rest::binary>> = body
with {:ok, conn} <- Mint.HTTP.stream_request_body(conn, ref, chunk) do
stream_body(conn, ref, rest)
end
end
endAs you see no need for using the HTTP2 module directly. No need to detect protocol. The Mint.HTTP module dispatches to the correct implementation itself. With this change we can keep |
| end | ||
|
|
||
| @impl true | ||
| def get_send_window(%__MODULE__{streaming_request: %{ref: ref}, sndbuf: sndbuf}, ref), |
There was a problem hiding this comment.
This is semantically very different to what what the HTTP2 module does. There is no concept of send windows in HTTP/1 so we should not expose it on this module.
| @behaviour Mint.Core.Conn | ||
|
|
||
| @opaque t() :: Mint.HTTP1.t() | Mint.HTTP2.t() | ||
| @type t() :: Mint.HTTP1.t() | Mint.HTTP2.t() |
There was a problem hiding this comment.
| @type t() :: Mint.HTTP1.t() | Mint.HTTP2.t() | |
| @type t() :: Mint.HTTP1.t() | Mint.HTTP2.t() | Mint.UnsafeProxy.t() |
| end | ||
|
|
||
| @impl true | ||
| def get_send_window(%__MODULE__{} = conn, ref), |
There was a problem hiding this comment.
get_window_size/2 is already public, I don't think we need a function that only calls min on top of that.
| def get_send_window(conn, ref), do: conn_apply(conn, :get_send_window, [conn, ref]) | ||
|
|
||
| @spec next_body_chunk_size(t(), term(), binary()) :: non_neg_integer() | ||
| def next_body_chunk_size(conn, ref, body), do: min(get_send_window(conn, ref), byte_size(body)) |
There was a problem hiding this comment.
I think this function is unnecessary.
Hey guys. Let me explain the problem I faced with. Below is the typical code for the sreaming body request
This code generates dialyzer warnings
It was discussed in the #380 and was left with a dirty workaround.
And people are using the workaround. And on the OTP-28 such trick in the k8s library leads to the infinite hanging during the PLT building phase.
But it's a legit issue. And it's totally worth to be fixed.
Opaqueness
Since the caller have to know the exact type for streaming, the
Mint.HTTP.t()cannot be opaque. And the caller cannot know the exact type in advance either, because it's not their decision, but the server's decision.Missing API for streaming
Another consequence of this code and dialyzer warning is the request body streaming API is not sufficient for using it without knowing the underlying protocol. But it's kinda designed to be such. To close the gap the current PR is introducing the missing piece - an ability to get the chunk size for the streaming.